-
-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix varlong serialization #853
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like our current (and your changes) of at least the readVarInt is wrong.
The if (size > 5)
and if (size > 35)
should become >=
. This is because of the & 0x80) = 0x80
bit in combination with the placement of the size check, as it currently allows varints with 6 bytes instead of the max 5. I personally btw prefer the old (length must be <= 5)
.
Maybe we can replace this with a do while
loop? I personally think that the current while condition makes it unnecessarily hard to read.
The implementation is correct, it has to be greater-than as a VarInt should be able to consist out of 35, or 70 bits to be able to fully represent 32 bit or 64 bit. (and therefore has to run 5 iterations, at the 6th iteration it should fail, which it doe) Using bits has just the slight advantage of not having to multiply by 7 each iteration, even though it's definitely not a bottleneck. |
You're right that the max length of a varint is 5 bytes to represent 4 bytes. But look at the condition in the while. This is why I recommend switching to a |
Some unit tests would be nice |
I mean you are theoretically right, but even the implementation Minecraft uses in both Bedrock and Java Edition post and prior Netty rewrite has this flaw. byte b;
do {
b = buf.readByte(); // will also do the 6th iteration
i |= (b & 127) << j++ * 7;
if (j > 5) {
throw new RuntimeException("VarInt too big");
}
} while (shouldContinueRead(b)); But I agree, it shouldn't consume the extra byte (even though it doesn't match the Vanilla impl) and I'll add a unit test |
The writeVarLong method seems to be also wrong But there are no serverbound varlong writes at least |
… simple tests to ensure functionality, VarInt/Long wider than 5/10 bytes will now take precedence over index out of bounds
readVarLong uses an int instead of a long for its value. This PR also removes the multiplications.
Original PR GeyserMC/PacketLib#48